-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upload to s3 feature added into interactive workflow #84
Conversation
@Sahiler Make sure you run the pre-commits in you're local environment, i.e. pip install pre-commit, then pre-commit run, I'll get that added to the requirements file for the demos repo so this triggers automatically when you add a git commit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure the pre-commits are run, we'll probably hold off on merging this to main I do have a Dev branch that we'll probably want to merge this to first for testing in the se-dev workspace, still need to make sure the rules for the dev workspace are setup correctly though
@@ -20,6 +20,7 @@ Interactive Workflow Demo is designed to guide you through the process of settin | |||
Before starting, ensure you have the following installed: | |||
- Python 3.6+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be 3.8 since that's the lowest version I think we support now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably assume that the user here knows that this requires Python. Do we even need a pre-requisites section?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's probably fair, I think in general we should assume relevant information for these demos outside of perfect specific functionality is familiar to whoever might look at it
from prefect import flow | ||
|
||
flow.from_source( | ||
source="https://github.com/Sahiler/interactive-workflow-demo.git", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll want to probably set this up in the dev branch for this repo first to do some fine tuning and then create/test the deployment here https://app.prefect.cloud/account/0ff44498-d380-4d7b-bd68-9b52da03823f/workspace/f579e720-7969-4ab8-93b7-2dfa784903e6/dashboard to start with before we merge into main
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments.
One bigger point is that reading over the README, it's not clear what this example is trying to accomplish. I understand what features are being used, but not what is happening, nor why I would use this pattern.
@@ -20,6 +20,7 @@ Interactive Workflow Demo is designed to guide you through the process of settin | |||
Before starting, ensure you have the following installed: | |||
- Python 3.6+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably assume that the user here knows that this requires Python. Do we even need a pre-requisites section?
Have a few readme updates and new s3 function to add in, converting to a draft in the meantime |
Moved away from using snowflake for the interactive workflow example as the output can often be unstructured text.
Two main reasons:
Next steps
I have a
generate_suggested_file_name
task that is currently a WIP, where currently the file name that is used is static in the rest of the workflow. I plan on debugging this flaky task, but currently the script is valid and demo-able. Additionally, will point the .deploy command to the prefect-demos repo main branch once the updated flow code gets merged in.